feat: numberfield announce pasted value#8604
Conversation
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
how best should we go about testing that this works with all kinds of locales/number formats? I was able to confirm the behavior with values like 3.000.000,25 and 1.000, but I guess it might be good enough that those pass since the parsing/formatting/validating is tested elsewhere
| let value = state.parser.parse(pastedText); | ||
| let reformattedValue = numberFormatter.format(value); | ||
| if (state.validate(reformattedValue)) { |
There was a problem hiding this comment.
Does there exist a case where the pastedText is considered valid but is unable to be parsed? The description of parse seems to suggest that might be the case, but digging to see if that is the case. Just concerned if there are any cases where we make it here but end up not announcing that we couldn't parse the value
| "numberField": "حقل رقمي" | ||
| "numberField": "حقل رقمي", | ||
| "pastedValue": "Pasted value: {value}", | ||
| "couldNotParseValue": "Could not understand value: {value}, try another format perhaps" |
There was a problem hiding this comment.
mega nit for more assertive message haha:
| "couldNotParseValue": "Could not understand value: {value}, try another format perhaps" | |
| "couldNotParseValue": "Could not understand value: {value}, please try another number format" |
|
Couldn't the original problem from #4967 still exist? If you typed the value (rather than pasted) and then blurred, and our parsing resulted in something different, it wouldn't get announced. Should we just announce the formatted value on blur instead of paste? |
Yeah, I was debating this, as the changes also exposed parse on the state object, which I wasn't thrilled with, in case the parser suddenly changed one render to another announcing in onBlur isn't quite right either, we don't know if the value is going to be accepted by a controlled component, so in that case the during paste was more "correct" we could do it in an effect after, but then it gets hard to know if we're just announcing typing (based on inputValue) or if we base it on value, then we may get no announcement if it failed to write a new number and reverted |
# Conflicts: # packages/@react-aria/numberfield/package.json # yarn.lock
|
Build successful! 🎉 |
Maybe something like this? let onBlur = () => {
let oldValue = inputRef.current.value;
flushSync(() => commit());
if (inputRef.current.value !== oldValue) {
announce(inputRef.current.value);
}
};Though I guess it would be better to detect if the actual number value changed rather than just the text, as it could have just re-formatted but still represent the same value. |
|
@devongovett I've updated the code, see if you like this better in terms of when announcements happen |
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Verified the behavior using 1,000 in de-DE and a couple other combinations of locales and number formats. I'm a bit unsure about the blur announce behavior since a VO user on keyboard would typically tab to a new element (I assume) and thus never hear the announcement as you noted. I was only able to hear it by clicking off the input onto the empty background.
The paste operation immediately formatting the value is fine I think though
# Conflicts: # packages/@react-aria/numberfield/package.json # yarn.lock
|
Build successful! 🎉 |
|
Build successful! 🎉 |
Any thoughts about the above? Dunno if making the announcement for VO is worth it if they won't hear it when focusing a new element, unless there is some other interaction I'm not thinking of here? |
They hear it when clicking off, so I think that's helpful at the very least. Yeah, otherwise not sure what we can do there. Possibly the work from #8776 will help with min/max at least, but the rest, i'm not sure. |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
still on the fence about the VO behavior but I guess it is an improvement that it at least announces on blur via click
# Conflicts: # packages/@react-aria/numberfield/package.json # yarn.lock
|
Build successful! 🎉 |
# Conflicts: # packages/react-aria-components/test/NumberField.test.js
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Sanity checked that behavior was still fine after the merge
|
Build successful! 🎉 |
| }); | ||
| // Note: this announcement will be skipped if the user is keyboard navigating to a new | ||
| // focusable element because that target will be announced instead, even though this is | ||
| // assertive. This is expected VO behaviour. |
There was a problem hiding this comment.
This seems like the common case though, so we need to delay it or something?
There was a problem hiding this comment.
I did some testing on the newest iOS+VO and MacOS+VO and this statement doesn't appear to be true anymore.
Also checked with Android + TalkBack on the latest OS there
|
Build successful! 🎉 |
# Conflicts: # packages/@react-aria/numberfield/package.json # yarn.lock
|
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:NumberFieldState NumberFieldState {
canDecrement: boolean
canIncrement: boolean
- commit: () => void
+ commit: (string) => void
commitValidation: () => void
decrement: () => void
decrementToMin: () => void
defaultNumberValue: number
increment: () => void
incrementToMax: () => void
inputValue: string
maxValue?: number
minValue?: number
numberValue: number
realtimeValidation: ValidationResult
resetValidation: () => void
setInputValue: (string) => void
setNumberValue: (number) => void
updateValidation: (ValidationResult) => void
validate: (string) => boolean
}@react-stately/color/@react-stately/color:ColorChannelFieldState ColorChannelFieldState {
canDecrement: boolean
canIncrement: boolean
colorValue: Color
- commit: () => void
+ commit: (string) => void
commitValidation: () => void
decrement: () => void
decrementToMin: () => void
defaultColorValue: Color | null
displayValidation: ValidationResult
increment: () => void
incrementToMax: () => void
inputValue: string
maxValue?: number
minValue?: number
numberValue: number
realtimeValidation: ValidationResult
resetValidation: () => void
setColorValue: (Color | null) => void
setInputValue: (string) => void
setNumberValue: (number) => void
updateValidation: (ValidationResult) => void
validate: (string) => boolean
}@react-stately/numberfield/@react-stately/numberfield:NumberFieldState NumberFieldState {
canDecrement: boolean
canIncrement: boolean
- commit: () => void
+ commit: (string) => void
commitValidation: () => void
decrement: () => void
decrementToMin: () => void
defaultNumberValue: number
increment: () => void
incrementToMax: () => void
inputValue: string
maxValue?: number
minValue?: number
numberValue: number
realtimeValidation: ValidationResult
resetValidation: () => void
setInputValue: (string) => void
setNumberValue: (number) => void
updateValidation: (ValidationResult) => void
validate: (string) => boolean
} |
devongovett
left a comment
devongovett
left a comment
There was a problem hiding this comment.
It seems to only announce sometimes when I tab away in Safari, not exactly sure why. But it's better than before.
5 participants
Closes #4967
Split out of #6520
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: